Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

orderBy should not reverse an array of objects #9566

Closed
wants to merge 1 commit into from
Closed

orderBy should not reverse an array of objects #9566

wants to merge 1 commit into from

Conversation

lukasolson
Copy link

When no predicate is provided, orderBy will reverse an array of objects.
This isn't ideal. It should err on the side of preserving the order.

It happens because, for objects, in the compare() function, v1 < v2 is
false, so the comparator function returns 1 (effectively moving v2
before v1).

When no predicate is provided, orderBy will reverse an array of objects.
This isn't ideal. It should err on the side of preserving the order.

It happens because, for objects, in the compare() function, v1 < v2 is
false, so the comparator function returns 1 (effectively moving v2
before v1).
@mary-poppins
Copy link

I'm sorry, but I wasn't able to verify your Contributor License Agreement (CLA) signature. CLA signature is required for any code contributions to AngularJS.

Please sign our CLA and ensure that the CLA signature email address and the email address in this PR's commits match.

If you signed the CLA as a corporation, please let us know the company's name.

Thanks a bunch!

PS: If you signed the CLA in the past then most likely the email addresses don't match. Please sign the CLA again or update the email address in the commit of this PR.
PS2: If you are a Googler, please sign the CLA as well to simplify the CLA verification process.

@lukasolson
Copy link
Author

Hmmm... I signed the CLA earlier today. The email address should match...

@caitp
Copy link
Contributor

caitp commented Oct 10, 2014

Your commit is authored with lolson@... --- the CLA signature verifier looks at the author email address. You can change it with git commit --amend --author="Name <email@address>" iirc (but might want to doublecheck those instructions)

@lukasolson
Copy link
Author

Thanks for being so patient with me. I just added that email to the CLA instead. Figured it would be easier. Let me know if that worked.

@mary-poppins
Copy link

CLA signature verified! Thank you!

Someone from the team will now triage your PR and it will be processed based on the determined priority (doc updates and fixes with tests are prioritized over other changes).

@lukasolson
Copy link
Author

Looking more into this, it's actually not a good fix for the given issue, because of the different ways browsers implement sorting. What should happen is this: When no predicate is provided, the array's contents are checked, and if the array contains objects, no sorting should be done.

@IgorMinar
Copy link
Contributor

you are correct. different vms compare objects differently so this PR is flawed.

I don't understand why you use orderBy filter in this situation at all.

@IgorMinar IgorMinar modified the milestones: Purgatory, 1.3.x Oct 16, 2014
@lukasolson
Copy link
Author

Sure, let me explain my use case, and maybe you could suggest a different way to do it.

I have a table, where each row corresponds to an object, like so:

<table>
    <thead>
        <tr>
            <th ng-click="predicate = 'name'">Name</th>
            <th ng-click="predicate = 'status'">Status</th>
            <th ng-click="predicate = 'date'">Date</th>
        </tr>
    </thead>
    <tbody>
        <tr ng-repeat="item in items | orderBy:predicate">
            <td>{{item.name}}</td>
            <td>{{item.status}}</td>
            <td>{{item.date}}</td>
        </tr>
    </tbody>
<table>

JSFiddle

Before a user clicks on a header cell, there is no predicate (it is undefined). I would expect it not to reorder the array, and I find it really strange that orderBy reverses the order of the array when no predicate is specified, but maybe it's a feature. :)

@m48u
Copy link

m48u commented Oct 21, 2014

We have a very similar use case for orderBy in tables like already descriped by @lukasolson
I provided a plnkr to demonstrate this behavior: http://plnkr.co/edit/WE4csJrhb4aQ8ZX1yNXs?p=preview
But we had several cases in production mode with large data set where order wasn't just reversed but in a complete new order. Maybe it is related to the _.identity functionality new to orderBy? Anyways

I would appreciate that orderBy donsn't order a given array as long as predicate and reverse order are undefined.

@m48u
Copy link

m48u commented Oct 21, 2014

maybe the easiest solution is to set orderBy defaults to "+ " (plus and space) instead of "+". This will work just fine
http://plnkr.co/edit/sxTb5h9vdkKAb08E0XgM?p=preview

@mshoaibsheikh
Copy link

I am also working with large dataset and with m48u fix, it started to work fine in all browsers except for Chrome where it shows wrong first row ( replacement of some random row from following rows ) when the data is first loaded using ng-repeat.Not sure , why it wouldn't work in Chrome only.I tested on FF,Safari,IE and Chrome

@juangabreil
Copy link
Contributor

I added a PR (#9747), please take a look.

@m48u
Copy link

m48u commented Oct 23, 2014

Thanks for the PR, but for me it seems that you're missing the point. By setting predicate to an empty string if not provided the order will end up reversed (same as "+"), which is IMHO wrong.
My idea was to set sortPredicate to ['+ '] (plus and space in line 17298 angular.js) instead of just "+" (plus sign).
This will solve most problems exept for that random first row in Chrome noticed by @mshoaibsheikh as well not only by me

caitp added a commit to caitp/angular.js that referenced this pull request Dec 3, 2014
…t provided

In ES262, there are two properties which are used to get a primitive value from an Object:

- valueOf() -- a method which returns a primitive value represented by the Object
- toString() -- a method which returns a string value representing the Object.

When comparing objects using relational operators, the abstract operation ToPrimitive(O, TypeHint) is used,
which will use these methods to retrieve a value.

This CL emulates the behaviour of ToPrimitive(), and ensures that no ordering occurs if the retrieved value
is identical.

This behaviour was previously used for Date objects, however it can be safely made generic as it applies to
all objects.

Closes angular#9566
Closes angular#9747
@caitp caitp closed this in 8bfeddb Dec 3, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants